Skip to content

build: unbreak --with-intl=system-icu build #28118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

build: unbreak --with-intl=system-icu build #28118

wants to merge 3 commits into from

Conversation

@bnoordhuis
Copy link
Member

Include directories are configured by the tools/icu/icu-*.gyp files.
The v8.gyp file doesn't need to add them and in fact the way it did
that breaks when building against an external copy of ICU.

Fixes: #28052

CI: https://ci.nodejs.org/job/node-test-commit/29244/

bnoordhuis added 3 commits June 7, 2019 11:38
@bnoordhuis
build: don't swallow pkg-config warnings
aa5e4cc
@bnoordhuis
build: fix icu-i18n pkg-config version check
1a62bc8 
The pkg_config() helper can either return a tuple of None values
(no pkg-config installed) and that was what the check was testing
for, but it can also return a tuple of empty strings when the
package isn't installed.
@bnoordhuis
build: unbreak --with-intl=system-icu build
167f4b0 
Include directories are configured by the tools/icu/icu-*.gyp files.
The v8.gyp file doesn't need to add them and in fact the way it did
that breaks when building against an external copy of ICU.

Fixes: nodejs#28052
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Jun 7, 2019
@bnoordhuis bnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 11, 2019
@Trott

This comment has been minimized.

Sign in to view
@bnoordhuis
Copy link
Member Author

Looks like the test failure is #27611.

05:59:40 not ok 2340 sequential/test-cpu-prof
05:59:40   ---
05:59:40   duration_ms: 480.90
05:59:40   severity: fail
05:59:40   exitcode: -15
05:59:40   stack: |-
05:59:40     timeout
05:59:40   ...

@Trott

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member

Trott commented Jun 13, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23865/

mhdawson
mhdawson approved these changes Jun 13, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott
Copy link
Member

Trott commented Jun 14, 2019

Landed in b614840...3cdd5a2

@Trott Trott closed this Jun 14, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 14, 2019
@bnoordhuis @Trott
build: don't swallow pkg-config warnings
16edec8 
PR-URL: nodejs#28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 14, 2019
@bnoordhuis @Trott
build: fix icu-i18n pkg-config version check
30f285d 
The pkg_config() helper can either return a tuple of None values
(no pkg-config installed) and that was what the check was testing
for, but it can also return a tuple of empty strings when the
package isn't installed.

PR-URL: nodejs#28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 14, 2019
@bnoordhuis @Trott
build: unbreak --with-intl=system-icu build
3cdd5a2 
Include directories are configured by the tools/icu/icu-*.gyp files.
The v8.gyp file doesn't need to add them and in fact the way it did
that breaks when building against an external copy of ICU.

Fixes: nodejs#28052
PR-URL: nodejs#28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Jun 16, 2019
Closed
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
@bnoordhuis @BridgeAR
build: don't swallow pkg-config warnings
e2ad364 
PR-URL: #28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
@bnoordhuis @BridgeAR
build: fix icu-i18n pkg-config version check
0931f7e 
The pkg_config() helper can either return a tuple of None values
(no pkg-config installed) and that was what the check was testing
for, but it can also return a tuple of empty strings when the
package isn't installed.

PR-URL: #28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
@bnoordhuis @BridgeAR
build: unbreak --with-intl=system-icu build
7d0ffee 
Include directories are configured by the tools/icu/icu-*.gyp files.
The v8.gyp file doesn't need to add them and in fact the way it did
that breaks when building against an external copy of ICU.

Fixes: #28052
PR-URL: #28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
Merged
targos pushed a commit that referenced this pull request Jun 18, 2019
@bnoordhuis @targos
build: don't swallow pkg-config warnings
02fdf5c 
PR-URL: #28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 18, 2019
@bnoordhuis @targos
build: fix icu-i18n pkg-config version check
eb89c06 
The pkg_config() helper can either return a tuple of None values
(no pkg-config installed) and that was what the check was testing
for, but it can also return a tuple of empty strings when the
package isn't installed.

PR-URL: #28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 18, 2019
@bnoordhuis @targos
build: unbreak --with-intl=system-icu build
6a2d8e2 
Include directories are configured by the tools/icu/icu-*.gyp files.
The v8.gyp file doesn't need to add them and in fact the way it did
that breaks when building against an external copy of ICU.

Fixes: #28052
PR-URL: #28118
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@kissofire kissofire mentioned this pull request Jul 22, 2022
Open
@wedataintelligence wedataintelligence mentioned this pull request Jul 22, 2022
Open
@ron-a20 ron-a20 mentioned this pull request Jul 22, 2022
Open
@Dmarch28 Dmarch28 mentioned this pull request Jul 22, 2022
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Jul 22, 2022
Open
@137717unity 137717unity mentioned this pull request Jul 22, 2022
Open
@baby636 baby636 mentioned this pull request Jul 22, 2022
Open
@molllyn1 molllyn1 mentioned this pull request Jul 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@richardlau richardlau richardlau approved these changes

@mhdawson mhdawson mhdawson approved these changes

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Undefined variable icu_path during configure when system-icu is set

7 participants

@bnoordhuis @nodejs-github-bot @Trott @refack @jasnell @richardlau @mhdawson